fix accuracy of logit
#99
Conversation
|
Plot of the error in ULPs after this change: 
The plotting happens after downsampling (which just takes the maximum value), which itself happens after smoothing (which also just takes the maximum, i.e., a sliding window maximum). Thus the downsampling and smoothing basically just remove the downward spikes of the noise, preserving the maximum error at the region around each point. |
oscardssmith has been reviewing the `ULPError` code on JuliaLang/julia#59087 So sync the code with the changes there.
test/common/ULPError.jl
Outdated
| # handle floating-point edge cases | ||
| if !(isfinite(accurate) && isfinite(approximate)) | ||
| accur_is_nan = isnan(accurate) | ||
| approx_is_nan = isnan(approximate) | ||
| if accur_is_nan || approx_is_nan | ||
| return if accur_is_nan === approx_is_nan | ||
| zero_return | ||
| else | ||
| inf_return | ||
| end | ||
| end | ||
| if isinf(approximate) | ||
| return if isinf(accurate) && (signbit(accurate) == signbit(approximate)) | ||
| zero_return | ||
| else | ||
| inf_return | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The case isfinite(approximate) and isinf(accurate) is not handled here. Generally, I think it would be clearer to just have a single flat chain of if/elseif statements to handle all cases, e.g.,
if isinan(accurate) || isnan(approximate)
return accur_is_nan === approx_is_nan ? zero_return : inf_return
elseif isinf(approximate)
return isinf(accurate) && (signbit(accurate) == signbit(approximate)) ? zero_return : inf_return
else
...
endThere was a problem hiding this comment.
The case
isfinite(approximate)andisinf(accurate)is not handled here.
Indeed. Only the cases not handled correctly by the general formula below are handled specially here.
Generally, I think it would be clearer to just have a single flat chain of if/elseif statements to handle all cases
That's basically what the current situation is, except that the branches are wrapped into the top-level single branch (if !(isfinite(accurate) && isfinite(approximate))) to prevent adverse effect on the performance of the general case.
|
Regarding reviewing It makes sense to keep the changes somewhat synchronized between the two PRs, thus the "update |
As suggested by devmotion on a PR to LogExpFunctions.jl: * JuliaStats/LogExpFunctions.jl#99
As suggested by devmotion on a PR to LogExpFunctions.jl: * JuliaStats/LogExpFunctions.jl#99
As suggested by devmotion on PR JuliaStats#99.
As suggested by devmotion on PR JuliaStats#99.
Specialization on the callable argument types should happen anyway.
|
In the PR to JuliaLang/julia, I also included some meta-tests to test Do you want those here, too? |
As suggested by devmotion on a PR to LogExpFunctions.jl: * JuliaStats/LogExpFunctions.jl#99
As suggested by devmotion on a PR to LogExpFunctions.jl: * JuliaStats/LogExpFunctions.jl#99
Fixes #98